perf: cache thread_id::current() in a #[thread_local] slot#30971
Conversation
|
Updated 1:29 AM PT - May 19th, 2026
❌ @Jarred-Sumner, your commit 1d1f2ed has 6 failures in
🧪 To try this PR locally: bunx bun-pr 30971That installs a local version of the PR into your bun-30971 --bun |
cf0ebee to
702135b
Compare
WalkthroughProject-wide lifetime parameterization and arena-backed vectors replace owned Vecs across AST, parser, bundler, and runtime. APIs and structs gain explicit lifetimes, allocator retagging is added, TLS caches introduced, and slice access standardized to as_slice/as_mut_slice. DevServer/ThreadPool plumbing and resolver borrow semantics updated. ChangesEnd-to-end arena/lifetime plumbing and allocator ownership
Suggested reviewers
|
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/ast/nodes.rs (1)
1099-1145:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
PartList<'a>now putsPartin the arena, butPartstill owns dropful global containers.
Partstill carries plainVec/MultiArrayList/ArrayHashMapfields (import_record_indices,dependencies,declared_symbols,symbol_uses,import_symbol_property_uses). OncePartlives insideArenaVec, those allocations survive every arena reset unless they are retagged to the AST heap or explicitly torn down first.As per coding guidelines, "Do not rely on Drop for correctness in arena-backed code (
bun_alloc::MimallocArena); explicitly free heap allocations, refcounts, or fds before arena reset".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ast/nodes.rs` around lines 1099 - 1145, The Part struct is being stored in an arena via PartList<'a> but still owns dropful heap containers (import_record_indices, dependencies, declared_symbols, symbol_uses, import_symbol_property_uses) which will survive an arena reset; change those fields to arena-backed containers or ensure they are explicitly freed before arena teardown. Concretely, replace plain Vec/ArrayHashMap/MultiArrayList types used in Part (import_record_indices, dependencies, declared_symbols, symbol_uses, import_symbol_property_uses) with their bun_alloc arena equivalents (or other AST-heap types) so their allocations are tracked by the same arena, or add a clear/teardown API (e.g., Part::teardown or a caller-side loop over PartList) that drops/frees/clears each of these fields before the bun_alloc::MimallocArena reset; update any code that constructs Parts to use the arena-aware factories or to call the teardown routine prior to resetting the arena.src/ast/ast_result.rs (1)
107-148:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
Ast<'a>is arena-resident, but some owned buffers are still on the global allocator.
empty_in()moves the outer AST lists into the arena, but fields such asexport_star_import_records: Box<[u32]>andtop_level_symbols_to_parts: ArrayHashMap<Ref, Vec<u32>>still rely onDrop/global allocation. BecauseAstis now bulk-freed with the arena, those buffers will leak on every reset.As per coding guidelines, "Watch the arena edge case in Rust: values in an Arena do not run Drop when arena resets - explicitly free/deref before arena resets, mirroring original Zig
deinit()order".src/bundler/Graph.rs (1)
20-29:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDo not expose a safe
Graphthat contains a danglingpool.
Graph::new()currently returns a fully safeGraphwhosepoolfield is initialized withNonNull::dangling(), and bothpool()andpool_mut()dereference it. That means any safe early read beforeBundleV2::initpatches the field is immediate UB.Suggested fix
-pub struct Graph<'a> { - pub pool: bun_ptr::BackRef<ThreadPool>, +pub struct Graph<'a> { + pool: Option<bun_ptr::BackRef<ThreadPool>>, pub heap: &'a ThreadLocalArena,- pool: bun_ptr::BackRef::from(NonNull::<ThreadPool>::dangling()), + pool: None,pub fn pool(&self) -> &ThreadPool { - self.pool.get() + self.pool + .as_ref() + .expect("Graph.pool used before initialization") + .get() }pub fn pool_mut(&mut self) -> &mut ThreadPool { - unsafe { self.pool.get_mut() } + unsafe { + self.pool + .as_mut() + .expect("Graph.pool used before initialization") + .get_mut() + } }Then set
graph.pool = Some(...)inBundleV2::init.Also applies to: 143-149, 169-197
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bundler/Graph.rs` around lines 20 - 29, Graph currently exposes a safe Graph with pool initialized as NonNull::dangling(), causing UB when pool()/pool_mut() dereference it; change Graph.pool from bun_ptr::BackRef<ThreadPool> to Option<bun_ptr::BackRef<ThreadPool>> (or otherwise make it an explicit nullable/backref wrapper), update Graph::new to set pool = None, adjust pool() and pool_mut() accessors to return Option<&ThreadPool>/Option<&mut ThreadPool> (or panic/handle None with a clear error) instead of dereferencing a dangling pointer, and then assign graph.pool = Some(...) from BundleV2::init after the real ThreadPool is allocated.src/bundler/Chunk.rs (1)
542-547:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winThread the
Graphlifetime parameter through these three method signatures.The
Graphstruct is now generic (Graph<'a>), but these three methods still reference&Graphwithout the lifetime parameter, which is a compile error. The parallel parameterlinker_graph: &LinkerGraph<'_>already has the lifetime syntax applied; updateparse_graphandgraphto match:Suggested fix
- parse_graph: &Graph, + parse_graph: &Graph<'_>,Apply to all three methods:
code()(line 542),code_standalone()(line 593), andcode_with_source_map_shifts()(line 638).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bundler/Chunk.rs` around lines 542 - 547, The methods code, code_standalone, and code_with_source_map_shifts must accept the generic Graph lifetime; update their signatures to use the Graph lifetime (e.g., change parameters typed as &Graph or graph: &Graph to &Graph<'_> or graph: &Graph<'_>) so the Graph<'a> definition is threaded through these methods, mirroring the existing linker_graph: &LinkerGraph<'_> usage; make this same change in all three method declarations to fix the compile error.src/bundler/LinkerContext.rs (1)
1901-1908:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard TLA recursion against non-JS targets.
Line 1906 indexes
ast_import_recordsfor every recursiveStmt/Requireedge, but this file already treats that column as shorter than the full file set inlink(). If a TLA module reaches a CSS/asset/no-AST file, this path can panic instead of skipping it.💡 Proposed fix
+ let next_source_index = record.source_index.get() as usize; + if next_source_index >= ast_import_records.len() { + continue; + } + let parent = self.validate_tla( record.source_index.get(), tla_keywords, tla_checks, input_files, - ast_import_records[record.source_index.get() as usize].as_slice(), + ast_import_records[next_source_index].as_slice(), meta_flags, ast_import_records, )?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bundler/LinkerContext.rs` around lines 1901 - 1908, The recursion in validate_tla can index ast_import_records for files that don't have ASTs (non-JS targets) causing a panic; update the code in validate_tla (call site using record.source_index.get() and ast_import_records) to first check that ast_import_records contains an entry for record.source_index.get() and that the entry is Some/has AST before indexing into it (e.g., use get(index).and_then(|r| r.as_slice()/Option) or bail/continue when out of range or None), matching the shorter column semantics enforced in link(), so TLA recursion simply skips CSS/asset/no-AST files instead of panicking.src/js_parser/lexer.rs (1)
2686-2705:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftFix the log lifetime enforcement; current signatures are unsound.
Lines 2687, 2702, and 2755 accept
log: &mut Logwith no lifetime bound, but the struct'slogfield doc (line 322–323) claims the constraint is "enforced by allinit*constructors." This is no longer true. Afterinit_without_reading(&mut log, source, arena), the type system permits safe code to either drop the log before the lexer is done (creating a dangling raw pointer) or take another&mut logwhile the lexer exists (creating an alias thatlog()will later materialize). The struct docstring (line 303–305) also incorrectly states "'ais the lifetime of the borrowedLog" when'anow applies only to source and arena. Restore the invariant by either reintroducing&'a mut Login the parameter signature, or add a separate'loglifetime withPhantomData<&'log mut Log>if source/arena must outlive the log.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/js_parser/lexer.rs` around lines 2686 - 2705, The constructors init_json and init_without_reading currently take log: &mut Log with no lifetime, which allows the caller to drop or reborrow the Log while the lexer holds a NonNull<Log> and is unsound; change the API to tie the Log's lifetime to the lexer by introducing an explicit lifetime (e.g. &'log mut Log) on those parameters or add a separate 'log lifetime to the Lexer type plus a PhantomData<&'log mut Log> field, and update the struct docstring (the comment about "'a is the lifetime of the borrowed Log") to reflect the new 'log lifetime so the invariant is enforced by the type system; ensure all init* constructors (init_json, init_without_reading, etc.) and the log field use the new 'log lifetime consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bun_alloc/ast_alloc.rs`:
- Around line 192-203: The current set_thread_heap preserves the bump cursor
when called with null, which is unsafe because MimallocArena may be dropped on
another thread; modify set_thread_heap so it also clears the per-thread bump
state when AST_HEAP is being set to null. Specifically, update the condition
around bump_reset() in set_thread_heap to call bump_reset() when heap.is_null()
OR when heap != BUMP_HEAP.get() (so it still preserves the cursor only for the
same non-null BUMP_HEAP), referencing AST_HEAP, BUMP_HEAP, BUMP_CUR, bump_reset,
and set_thread_heap to locate the change.
In `@src/bundler/bundle_v2.rs`:
- Line 2612: The append call currently ignores failures (let _ =
self.graph.ast.append(JSAst::empty_in(self.graph.heap))) which can desynchronize
graph.ast from input_files; change this to handle the Result by converting
allocation errors into a controlled OOM crash using bun_core::handle_oom or the
.unwrap_or_oom() helper (i.e., call self.graph.ast.append(...).unwrap_or_oom()
or pass the Result into bun_core::handle_oom) so allocation failures abort
deterministically; apply the same fix to the other graph.ast.append(...) sites
(the similar calls at the other noted locations) to ensure all AST slot
allocations are handled consistently.
- Around line 6893-6895: The code tries to clone a bun_ast::Source via
empty_html_file_source.clone() when constructing a crate::Graph::InputFile
(fake_input_file), but bun_ast::Source isn't Clone; instead rebuild the Source
struct field-by-field like you do in enqueue_server_component_generated_file and
reserve_source_indexes_for_bake: create a new bun_ast::Source instance by
copying each field (e.g., source text, span, kind, etc.) from
empty_html_file_source into the new Source and use that new instance in
Graph::InputFile's source field so the code compiles without requiring Clone.
In `@src/bundler/HTMLImportManifest.rs`:
- Around line 57-61: The remaining uses of the bare Graph type must be changed
to the generic form with the lifetime/type parameters: update the function
signatures and impl header for write_escaped_json and write to accept/return
Graph<'a> (or the correct Graph<'a, T> form used elsewhere) instead of bare
Graph, and update the type argument in from_ref::<Graph>(graph) to
from_ref::<Graph<'a>>(graph) (or from_ref::<Graph<'a, T>>(graph) if Graph has a
second type parameter); ensure any function-level generics or impl lifetimes
match the HTMLImportManifest<'a> lifetime so all Graph references compile.
In `@src/bundler/linker_context/scanImportsAndExports.rs`:
- Around line 564-577: The cached identifier vs formatted identifier diverge for
strict-mode reserved words causing inconsistent symbol names; fix by ensuring
the same remapping is applied in both paths—either force-populate the cache
before formatting (call the method that triggers
MutableString::ensure_valid_identifier() so source.identifier_name is filled)
or, when using source.fmt_identifier() (the FormatValidIdentifier::fmt() path),
run the same strict_mode_reserved_word_remap()/ensure_valid_identifier logic on
the produced bytes so the ident used for init_/exports_/module_ symbol
construction is identical regardless of whether source.identifier_name was
pre-populated; update the block around ident, ident_scratch and the call sites
so they always use the remapped identifier.
In `@src/bundler/LinkerGraph.rs`:
- Around line 629-633: The loop over self.reachable_files currently indexes into
self.ast.items_import_records_mut() without guaranteeing a corresponding row,
which can panic for non-JS sources; modify the loop that uses
import_records_list[source_id.get() as usize] so it first computes let idx =
source_id.get() as usize and skips if idx >= import_records_list.len() (or
otherwise check that import_records_list has an entry for that source), i.e.,
continue when out of bounds (or when the source is a non-JS/asset if you prefer
that check), before calling .as_mut_slice() and rewriting
import_record::List<'_>.
In `@src/bundler/ThreadPool.rs`:
- Around line 423-435: The fast path in get_worker uses TLS_WORKER without
guarding against Worker::deinit() freeing the boxed Worker, causing a UAF;
update the teardown path to invalidate the cached TLS entry instead of leaving a
stale pointer. Concretely, in Worker::deinit (or where workers are
freed/deinit_soon) clear or bump TLS_WORKER for the current thread (e.g., set
its generation to a non-matching value or null the worker pointer) so
get_worker()'s generation check cannot succeed on a freed Worker; apply the same
invalidation for the other fast-path accessor noted around the 493-500 region
(the other get_* method that uses TLS_WORKER and get_worker_slow) so both fast
paths are protected.
In `@src/bundler/ungate_support.rs`:
- Line 608: The comment above the type alias JSAst should be updated to reflect
that it is now lifetime-generic (pub type JSAst<'a> = crate::BundledAst<'a>)
rather than being erased to 'static; edit the note that mentions "'static" to
instead explain that JSAst carries the lifetime parameter through to BundledAst
and is not implicitly 'static, clarifying any ownership/borrow implications and
removing the stale contract language referencing erasure to 'static.
In `@src/js_parser/parse/parse_entry.rs`:
- Around line 318-320: Restore the lifetime tie on the logger parameter of init
by changing the signature back to accept log: &'a mut bun_ast::Log so the input
reference lifetime matches Parser<'a>'s stored non-null pointer; update the init
function signature (init) to use &'a mut bun_ast::Log, ensuring
Parser<'a>::self.log (NonNull<bun_ast::Log>) is derived from that &'a mut
reference and that log_mut()’s safety claim is now enforced by the type system
rather than only by documentation.
In `@src/resolver/resolver.rs`:
- Around line 3221-3227: The global-cache branch that sets the local variable
conditions currently maps ImportKind::Require and RequireResolve to
self.opts.conditions.require and everything else to self.opts.conditions.import,
which omits ImportKind::At and ImportKind::AtConditional and causes CSS
resolution divergence; update the match arms where conditions is assigned (the
blocks that pattern-match ast::ImportKind) to treat ImportKind::At and
ImportKind::AtConditional like the local node_modules path by returning
self.opts.conditions.style for those variants (apply the same change to both
match blocks around the global-cache logic).
In `@src/runtime/bake/DevServer.rs`:
- Around line 279-283: The CurrentBundle struct currently declares pub heap:
Box<bun_alloc::MimallocArena> before fields that borrow from that arena (e.g.,
pub start_data: bundler::bundle_v2::DevServerInput, requests,
resolution_failure_entries, promise), which causes the arena to be dropped too
early; move the heap field to be the last field in CurrentBundle so the arena is
dropped after start_data, requests, resolution_failure_entries, and promise,
ensuring finalize_bundle and bv2.deinit_without_freeing_arena() can safely clean
up arena-backed allocations.
---
Outside diff comments:
In `@src/ast/nodes.rs`:
- Around line 1099-1145: The Part struct is being stored in an arena via
PartList<'a> but still owns dropful heap containers (import_record_indices,
dependencies, declared_symbols, symbol_uses, import_symbol_property_uses) which
will survive an arena reset; change those fields to arena-backed containers or
ensure they are explicitly freed before arena teardown. Concretely, replace
plain Vec/ArrayHashMap/MultiArrayList types used in Part (import_record_indices,
dependencies, declared_symbols, symbol_uses, import_symbol_property_uses) with
their bun_alloc arena equivalents (or other AST-heap types) so their allocations
are tracked by the same arena, or add a clear/teardown API (e.g., Part::teardown
or a caller-side loop over PartList) that drops/frees/clears each of these
fields before the bun_alloc::MimallocArena reset; update any code that
constructs Parts to use the arena-aware factories or to call the teardown
routine prior to resetting the arena.
In `@src/bundler/Chunk.rs`:
- Around line 542-547: The methods code, code_standalone, and
code_with_source_map_shifts must accept the generic Graph lifetime; update their
signatures to use the Graph lifetime (e.g., change parameters typed as &Graph or
graph: &Graph to &Graph<'_> or graph: &Graph<'_>) so the Graph<'a> definition is
threaded through these methods, mirroring the existing linker_graph:
&LinkerGraph<'_> usage; make this same change in all three method declarations
to fix the compile error.
In `@src/bundler/Graph.rs`:
- Around line 20-29: Graph currently exposes a safe Graph with pool initialized
as NonNull::dangling(), causing UB when pool()/pool_mut() dereference it; change
Graph.pool from bun_ptr::BackRef<ThreadPool> to
Option<bun_ptr::BackRef<ThreadPool>> (or otherwise make it an explicit
nullable/backref wrapper), update Graph::new to set pool = None, adjust pool()
and pool_mut() accessors to return Option<&ThreadPool>/Option<&mut ThreadPool>
(or panic/handle None with a clear error) instead of dereferencing a dangling
pointer, and then assign graph.pool = Some(...) from BundleV2::init after the
real ThreadPool is allocated.
In `@src/bundler/LinkerContext.rs`:
- Around line 1901-1908: The recursion in validate_tla can index
ast_import_records for files that don't have ASTs (non-JS targets) causing a
panic; update the code in validate_tla (call site using
record.source_index.get() and ast_import_records) to first check that
ast_import_records contains an entry for record.source_index.get() and that the
entry is Some/has AST before indexing into it (e.g., use get(index).and_then(|r|
r.as_slice()/Option) or bail/continue when out of range or None), matching the
shorter column semantics enforced in link(), so TLA recursion simply skips
CSS/asset/no-AST files instead of panicking.
In `@src/js_parser/lexer.rs`:
- Around line 2686-2705: The constructors init_json and init_without_reading
currently take log: &mut Log with no lifetime, which allows the caller to drop
or reborrow the Log while the lexer holds a NonNull<Log> and is unsound; change
the API to tie the Log's lifetime to the lexer by introducing an explicit
lifetime (e.g. &'log mut Log) on those parameters or add a separate 'log
lifetime to the Lexer type plus a PhantomData<&'log mut Log> field, and update
the struct docstring (the comment about "'a is the lifetime of the borrowed
Log") to reflect the new 'log lifetime so the invariant is enforced by the type
system; ensure all init* constructors (init_json, init_without_reading, etc.)
and the log field use the new 'log lifetime consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2c7a6f0e-ce74-4932-b91e-22a55302d95e
📒 Files selected for processing (71)
src/ast/ast_result.rssrc/ast/import_record.rssrc/ast/nodes.rssrc/ast/symbol.rssrc/bun_alloc/MimallocArena.rssrc/bun_alloc/ast_alloc.rssrc/bun_alloc/lib.rssrc/bun_core/lib.rssrc/bun_core/thread_id.rssrc/bundler/AstBuilder.rssrc/bundler/Chunk.rssrc/bundler/Graph.rssrc/bundler/HTMLImportManifest.rssrc/bundler/LinkerContext.rssrc/bundler/LinkerGraph.rssrc/bundler/ParseTask.rssrc/bundler/ServerComponentParseTask.rssrc/bundler/ThreadPool.rssrc/bundler/barrel_imports.rssrc/bundler/bundle_v2.rssrc/bundler/bundled_ast.rssrc/bundler/cache.rssrc/bundler/lib.rssrc/bundler/linker.rssrc/bundler/linker_context/MetafileBuilder.rssrc/bundler/linker_context/StaticRouteVisitor.rssrc/bundler/linker_context/computeCrossChunkDependencies.rssrc/bundler/linker_context/convertStmtsForChunk.rssrc/bundler/linker_context/convertStmtsForChunkForDevServer.rssrc/bundler/linker_context/doStep5.rssrc/bundler/linker_context/findAllImportedPartsInJSOrder.rssrc/bundler/linker_context/findImportedCSSFilesInJSOrder.rssrc/bundler/linker_context/findImportedFilesInCSSOrder.rssrc/bundler/linker_context/generateCodeForFileInChunkJS.rssrc/bundler/linker_context/generateCodeForLazyExport.rssrc/bundler/linker_context/generateCompileResultForHtmlChunk.rssrc/bundler/linker_context/postProcessJSChunk.rssrc/bundler/linker_context/renameSymbolsInChunk.rssrc/bundler/linker_context/scanImportsAndExports.rssrc/bundler/transpiler.rssrc/bundler/ungate_support.rssrc/css/css_parser.rssrc/css/dependencies.rssrc/css/printer.rssrc/css/properties/custom.rssrc/css/selectors/selector.rssrc/css/values/ident.rssrc/css/values/url.rssrc/js_parser/lexer.rssrc/js_parser/lib.rssrc/js_parser/p.rssrc/js_parser/parse/parse_entry.rssrc/js_parser/parser.rssrc/js_printer/lib.rssrc/jsc/AsyncModule.rssrc/jsc/RuntimeTranspilerStore.rssrc/jsc/bindings/wtf-bindings.cppsrc/jsc/lib.rssrc/paths/resolve_path.rssrc/resolver/lib.rssrc/resolver/package_json.rssrc/resolver/resolver.rssrc/runtime/api/JSTranspiler.rssrc/runtime/api/js_bundle_completion_task.rssrc/runtime/bake/DevServer.rssrc/runtime/bake/dev_server/incremental_graph.rssrc/runtime/cli/create/SourceFileProjectGenerator.rssrc/runtime/cli/repl.rssrc/runtime/cli/test/ChangedFilesFilter.rssrc/runtime/jsc_hooks.rssrc/runtime/test_runner/snapshot.rs
| pub fn set_thread_heap(heap: *mut mimalloc::Heap) { | ||
| AST_HEAP.set(heap); | ||
| bump_reset(); | ||
| // Keep the cursor when re-entering the heap that owns it (the bundler's | ||
| // per-task `push()`/`pop()` always passes the same per-worker arena). Only | ||
| // discard when switching to a *different* non-null heap — the chunk then | ||
| // belongs to the wrong owner. Clearing to null is a no-op for the cursor | ||
| // (no allocs happen while `AST_HEAP` is null), so it survives the | ||
| // `pop()→push()` round-trip. Heap destruction must call [`bump_reset`] | ||
| // explicitly to defend against address reuse. | ||
| if !heap.is_null() && heap != BUMP_HEAP.get() { | ||
| bump_reset(); | ||
| } |
There was a problem hiding this comment.
Don't preserve the bump cursor across set_thread_heap(null) while arenas are Send.
BUMP_HEAP/BUMP_CUR are thread-local, but MimallocArena can be reset or dropped on a different thread. After a pop on thread A the cursor now survives there; destroying the heap on thread B only clears B's TLS, not A's. If mi_heap_new() later reuses that address on A, the pointer-equality fast path at Lines 201-203 keeps a freed chunk alive and heap_alloc() can hand out UAF pointers.
Safe fallback
pub fn set_thread_heap(heap: *mut mimalloc::Heap) {
AST_HEAP.set(heap);
- if !heap.is_null() && heap != BUMP_HEAP.get() {
+ if heap.is_null() || heap != BUMP_HEAP.get() {
bump_reset();
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/bun_alloc/ast_alloc.rs` around lines 192 - 203, The current
set_thread_heap preserves the bump cursor when called with null, which is unsafe
because MimallocArena may be dropped on another thread; modify set_thread_heap
so it also clears the per-thread bump state when AST_HEAP is being set to null.
Specifically, update the condition around bump_reset() in set_thread_heap to
call bump_reset() when heap.is_null() OR when heap != BUMP_HEAP.get() (so it
still preserves the cursor only for the same non-null BUMP_HEAP), referencing
AST_HEAP, BUMP_HEAP, BUMP_CUR, bump_reset, and set_thread_heap to locate the
change.
| .put(path_slice, source_index.get()) | ||
| .expect("oom"); | ||
| let _ = self.graph.ast.append(JSAst::empty()); // OOM/capacity: Zig aborts; port keeps fire-and-forget | ||
| let _ = self.graph.ast.append(JSAst::empty_in(self.graph.heap)); // OOM/capacity: Zig aborts; port keeps fire-and-forget |
There was a problem hiding this comment.
Don't ignore graph.ast.append(...) failures.
Each of these sites immediately treats the AST row as allocated. If append fails, the code can keep going with a source_index that has no matching AST slot, which desynchronizes graph.ast from input_files/scheduled tasks and breaks later indexed access. Handle the allocation failure explicitly here instead of fire-and-forget.
As per coding guidelines, use bun_core::handle_oom or .unwrap_or_oom() extension to convert Result<T, AllocError> into controlled crash instead of OOM panic unwinding into FFI.
Also applies to: 2728-2728, 3248-3248, 3565-3565, 3619-3619, 3734-3734, 4707-4707, 6681-6681
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/bundler/bundle_v2.rs` at line 2612, The append call currently ignores
failures (let _ = self.graph.ast.append(JSAst::empty_in(self.graph.heap))) which
can desynchronize graph.ast from input_files; change this to handle the Result
by converting allocation errors into a controlled OOM crash using
bun_core::handle_oom or the .unwrap_or_oom() helper (i.e., call
self.graph.ast.append(...).unwrap_or_oom() or pass the Result into
bun_core::handle_oom) so allocation failures abort deterministically; apply the
same fix to the other graph.ast.append(...) sites (the similar calls at the
other noted locations) to ensure all AST slot allocations are handled
consistently.
The bundler's Worker::get(ctx) calls bun_threading::current_thread_id() once per scheduled task to look up the thread's Worker in the pool's assignment map. That routes to bun_core::thread_id::current(), which made a fresh gettid()/pthread_threadid_np()/GetCurrentThreadId() syscall on every call. A 19 K-module bundle (rolldown apps/10000) schedules ~5.7 tasks per module (parse, line-offset table, quoted source contents, compile-result generation, link step 5), so it paid ~109 K gettid syscalls vs. the Zig version's ~129 - about 36% of the build's total syscall time. Zig's std.Thread.getCurrentId() doesn't have this cost: LinuxThreadImpl reads a threadlocal var tls_thread_id set once at thread start (vendor/zig/lib/std/Thread.zig:841,885). Cache the result in a bare #[thread_local] Cell<ThreadId> slot so subsequent calls are a single TLS load with no LocalKey initialization branch or destructor registration. Lazy rather than set-at-spawn so threads not started through Bun's pool (FFI callbacks, the main thread) still get a valid ID; 0 is the unset sentinel since kernel TIDs and Win32/Darwin thread IDs are nonzero.
…e lexer log lifetime
…p (WIP: ~60 errors)
…ParseTask - ParseResult/ParseOptions carry the arena lifetime; cold loader fns take &'a Arena - ResolveImportRecordCtx/ImportInfo take &[ImportRecord] (allocator-agnostic) - arena-allocate parser Source so Ast<'bump> isn't pinned to the stack frame - ArenaVec call sites use std slice/index ops instead of BabyListExt - Worker::arena() returns &'static (centralises the per-task detach)
…allers - ParseOptions splits arena lifetime from short-lived input borrows - DevServer CurrentBundle owns the boxed arena bv2.graph.heap borrows - JSTranspiler/jsc_hooks reuse the existing per-call arena erasure for ParseOptions.arena - AsyncModule/js_bundle_completion_task adapt to borrowed Graph.heap
…LinkerGraph::load Per-file PartList/import_record::List buffers come from per-worker mi_heaps, which mi_heap_malloc cannot grow from the linker thread. Bitwise-move them into the linker-thread arena alongside the existing symbol-map copy so add_part_to_file etc. can append. The parse-side alias keeps the original handle (slab-freed without element drop, same as before).
…ager re-seat Replace LinkerGraph::load's reseat_col! (Vec::with_capacity_in + memcpy for every file's parts/import_records) with bun_alloc::transfer_arena — swap the ArenaVec's &Arena handle from the per-worker mi_heap to the bundle-thread heap via ManuallyDrop + from_raw_parts_in. Only files the linker actually grows pay a (lazy) cross-heap mi_heap_realloc migration. <&MimallocArena as Allocator>::deallocate is heap-agnostic mi_free, and grow is mi_heap_realloc_aligned(dst, ptr, ..) — alloc on dst, mi_free old — so retagging preserves the single-thread-alloc contract while matching Zig's BabyList.transferOwnership (release no-op there because BabyList is allocator-erased; Vec<T,&Arena> stores the handle, hence the swap). Drop the post-step-5 take_ast_ownership call: do_step_5 only pushes to global-allocator Vecs (Dependency, local_parts_with_uses), never to the arena-backed PartList/import-record columns. rolldown apps/10000 (--production --sourcemap, 8 runs): wall 520ms -> 501ms RSS 947MB -> 896MB vs bun-1.3.14: 433ms / 647MB
… (thread, pool) Keyed on a monotonic per-pool generation (not pool address — Bun.build() reuse makes pointer identity ABA). Drops the workers_assignments lock from the ~100K-per-build hot path to ~nthreads acquisitions; perf attributed ~97% of the build's futex traffic to the per-call lock on the rolldown 19K-module benchmark. Also drops the dead HELP_CATCH_MEMORY_ISSUES blocks in Worker::get/unget and the stale bumpalo references in this file.
…u32, _> source_index keys are dense 0..module_count and this map is probed once per import inside on_parse_task_complete (the main-thread parse-phase throughput limiter). Replaces hash+probe with direct index.
…resolve_without_symlinks
…pping through Ast::empty_in + init
…red_imports The Zig original used a 4096-byte stack-fallback ArrayHashMap; the Rust port heap-allocated an ArrayHashMap<u32, ()> per parsed file. Swap to AutoBitSet sized to file_import_records.len() — it stays in its inline 2-word Static arm for the typical <128-record file and is O(1) word ops to set/probe instead of hash+probe.
…ep-cloning Zig's Entry.data holds slices/pointers so its by-value return is a shallow few-word copy. The Rust port made EntryData own boxed slices/Vecs, so entry.value.clone() and exports.clone() deep-copied the entire conditions subtree on every resolve. Return Option<&Entry> from value_for_key and match exports by reference in resolve_exports; resolve_target already takes &Entry so callers just drop the local sentinel and pass the borrow through.
…ate on mi_heap_destroy set_thread_heap() previously bump_reset() unconditionally, so the bundler's per-task Worker::get → ASTMemoryAllocator::push() abandoned a 16 KB bump chunk on every task (~70K tasks × 16 KB ≈ 1.1 GB into never-reset worker arenas, mostly <500 B used per chunk). Now tracks BUMP_HEAP (the chunk's owner) and keeps the cursor when re-entering that same heap; MimallocArena reset/Drop calls bump_invalidate_heap() before mi_heap_destroy so a recycled mi_heap_t* slot can't ABA-match a stale cursor. rolldown apps/10000 (20K modules): peak RSS 895 → 607 MB, wall 466 → 448 ms.
Port of BabyList(T) (collections/baby_list.zig: ptr + u32 len + u32 cap = 16 B). The Rust port keeps the &'a MimallocArena handle inline for lifetime checking, so 24 B vs the previous Vec<T, &'a MimallocArena> = 32 B. Growth/free route through <&MimallocArena as Allocator> as before; transfer_arena is now a single allocator-field swap. Covers the existing call surface (push/pop/insert/swap_remove/append/ prepend_from/truncate/extend/drain/leak/allocator/IntoIterator) plus io::Write for BabyVec<'_, u8>. ArenaVecExt is implemented for both BabyVec and the legacy Vec<T,&Arena> so ArenaString stays on usize-backed Vec<u8>.
702135b to
e21d0b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/ast/ast_result.rs (1)
23-61:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMove the remaining owned
Astfields off the global allocator.Now that
Ast<'a>itself is arena-backed, its destructor no longer runs on arena reset.export_star_import_records,top_level_symbols_to_parts(including the innerVec<u32>s), andts_enums' innerStringHashMaps are still drop-managed/global-backed, so populated ASTs will leak those buffers every time the arena is recycled. These fields need arena-backed storage too, or an explicit teardown before reset. As per coding guidelines, "Do not rely onDropfor correctness in arena-backed code (values inbun_alloc::MimallocArenado not runDropwhen the arena resets) — free resources explicitly before the arena resets."Also applies to: 107-148
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ast/ast_result.rs` around lines 23 - 61, The Ast<'a> struct still contains heap-owned fields (export_star_import_records, top_level_symbols_to_parts including its inner Vec<u32>s, and ts_enums with their StringHashMaps) that rely on Drop and therefore leak when the arena is reset; switch those fields to arena-backed collections (e.g., replace Vec/HashMap/StringHashMap with the project's arena equivalents or Store/PartList-style types used elsewhere) or add an explicit teardown method (e.g., Ast::free_arena_owned_fields or Ast::teardown) that manually clears/frees export_star_import_records, top_level_symbols_to_parts (iterate and clear inner Vec<u32>s), and ts_enums maps before the arena reset; update constructors and any code that mutates these fields to allocate into the arena variants and ensure the new teardown is called where the arena is reset.src/bundler/LinkerContext.rs (1)
1898-1910:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard recursive TLA walks before indexing
ast_import_records.The top-level loop skips sources past
import_records_len, but Line 1907 recurses intorecord.source_indexwithout re-checking that invariant. A TLA-bearing JS module that statically imports a non-AST target here will panic during validation.💡 Minimal fix
for (import_record_index, record) in import_records.iter().enumerate() { if Index::is_valid(record.source_index) && (record.kind == ImportKind::Require || record.kind == ImportKind::Stmt) { + let imported_index = record.source_index.get() as usize; + if imported_index >= ast_import_records.len() { + continue; + } + let parent = self.validate_tla( - record.source_index.get(), + imported_index as u32, tla_keywords, tla_checks, input_files, - ast_import_records[record.source_index.get() as usize].as_slice(), + ast_import_records[imported_index].as_slice(), meta_flags, ast_import_records, )?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bundler/LinkerContext.rs` around lines 1898 - 1910, The loop over import_records calls validate_tla with ast_import_records[record.source_index.get() as usize].as_slice() without re-checking that record.source_index points to an AST-bearing entry; to fix, add a guard before indexing (e.g. ensure record.source_index.get() as usize < ast_import_records.len() or compare against import_records_len) and skip or handle non-AST targets so validate_tla is only invoked with a valid slice; update the code paths around the for loop and the validate_tla(...) call to use a checked access (or early continue) when the source_index is out of bounds.src/bundler/transpiler.rs (1)
1779-1848:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve
runtime_transpiler_cachein theAlreadyBundledresult.Line 1846 hardcodes
None, even thoughrtc_ptrwas computed specifically to be forwarded into the returnedParseResult. That drops cache propagation for theAlreadyBundledfast path.Suggested fix
js_ast::Result::AlreadyBundled(already_bundled) => ParseResult { // TODO(port): Zig used `undefined` for ast here. ast: bun_ast::Ast::empty_in(arena), already_bundled: match already_bundled { js_ast::AlreadyBundled::Bun => AlreadyBundled::SourceCode, @@ source: source.clone(), loader, input_fd, pending_imports: Default::default(), - runtime_transpiler_cache: None, + runtime_transpiler_cache: rtc_ptr, empty: false, source_contents_backing: source_backing, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bundler/transpiler.rs` around lines 1779 - 1848, The AlreadyBundled branch of js_ast::Result constructs a ParseResult but currently sets runtime_transpiler_cache to None, dropping the previously computed rtc_ptr; update the ParseResult construction (in the js_ast::Result::AlreadyBundled arm) to forward the computed rtc_ptr into runtime_transpiler_cache (use the same value/name used earlier, e.g., rtc_ptr or the variable that holds the runtime transpiler cache) so cache propagation is preserved for the AlreadyBundled fast path; ensure the field type matches (wrap/clone/transfer as needed) when assigning to runtime_transpiler_cache in the ParseResult returned alongside ast, already_bundled, source, loader, input_fd, etc.src/js_parser/lexer.rs (1)
2686-2705:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftRestore a lifetime tie for
logor make these constructors unsafe.All three public constructors (
init,init_json,init_without_reading) now accept&mut Loginstead of&'a mut Log, but the struct field still stores a rawNonNull<Log>and thelog()method re-materializes&mut Logfrom it. The field documentation andlog()safety comment both assume the original lifetime constraint exists, but the type system no longer enforces it. Safe code can now create a lexer from a stack-localLog, let it drop, and hit a use-after-free on the nextlog()call.Either thread a separate
'_loglifetime through the struct and constructors, or move these constructors into an unsafe/internal API.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/js_parser/lexer.rs` around lines 2686 - 2705, The public constructors (init, init_json, init_without_reading) accept &mut Log but the struct stores NonNull<Log> and log() reconstitutes &mut Log, creating a potential use-after-free; restore the borrow by changing the constructors to accept &'a mut Log (or add an explicit '_log lifetime on the struct) so the compiler enforces that the Log outlives the lexer, or else mark these constructors unsafe/internal and document the safety requirements; update the signatures of init, init_json, and init_without_reading and any callers accordingly, and adjust the field/lifetime comments to match the chosen approach.src/runtime/api/JSTranspiler.rs (1)
801-825:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix the lifetime contract in
get_parse_result()—ParseResult<'static>is unsound on the non-REPL path.At line 1264,
get_parse_result()returnsParseResult<'static>, but on the non-REPL path (line ~1285)processed_codeis just an alias to the caller'scodeparameter. The AST inParseResultis generic over a lifetime because it borrows from the source (this is standard parser behavior), so the returned result cannot claim to be'staticwhen its source bytes come from the caller's stack.Either ensure both code paths copy non-REPL bytes into the arena before creating the source, or revert the return type to generic lifetime so the contract matches reality. Lines 1392 and 1583 consume the result inline, but the type signature will invite misuse in the future.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/api/JSTranspiler.rs` around lines 801 - 825, get_parse_result() currently lies by returning ParseResult<'static> while on the non-REPL path processed_code is an alias to the caller's code buffer, so the AST borrows non-'static data; fix by either (A) copying non-REPL bytes into the allocated arena before creating the arena_ref/source so the AST truly borrows arena-owned data (update the non-REPL branch where processed_code is set and ensure arena_ref.alloc is used for the bytes), or (B) change the function signature and ParseResult usage to be lifetime-generic (e.g., ParseResult<'a>) so the return reflects the caller-owned lifetime and update callers of get_parse_result() accordingly (adjust types at call sites that consume ParseResult). Ensure modifications reference get_parse_result, ParseResult<'static>, processed_code, source, and arena/arena_ref so the borrowed lifetimes are consistent.src/runtime/cli/repl.rs (1)
1852-1866: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winImprove variable naming in
transform_for_repl; thearenabinding is shadowed.Line 1852 rebinds
arenato a different type (&'a MimallocArenafrom the symbols allocator), shadowing the parse arena created at line 1783. While this doesn't cause a functional error—both types implement the correct allocator interface forprint_ast—the shadowing makes the code harder to follow and verify.Rename the parse arena to
parse_arenaand the symbol allocator tosymbols_allocto clarify intent:Suggested fix
- let arena = bun_alloc::Arena::new(); + let parse_arena = bun_alloc::Arena::new(); @@ - &arena, + &parse_arena, @@ - let arena = *ast.symbols.allocator(); + let symbols_alloc = *ast.symbols.allocator(); let symbols_map = bun_ast::symbol::Map::init_with_one_list( - core::mem::replace(&mut ast.symbols, bun_alloc::ArenaVec::new_in(arena)) + core::mem::replace(&mut ast.symbols, bun_alloc::ArenaVec::new_in(symbols_alloc)) .into_iter() .collect(), ); @@ - &arena, + &parse_arena,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/cli/repl.rs` around lines 1852 - 1866, The code in transform_for_repl rebinds the variable arena (first used as the parse arena) when assigning from ast.symbols.allocator(), which shadows the original and reduces clarity; rename the original parse arena binding to parse_arena and rename the allocator binding to symbols_alloc (or similar) wherever they are declared and used (e.g., the initial parse arena declaration around line ~1783 and the later let arena = *ast.symbols.allocator(); used by print_ast and bun_alloc::ArenaVec::new_in) and update all references in this function (including calls to bun_js_printer::print_ast, bun_alloc::ArenaVec::new_in, and symbol Map initialization) so the two distinct allocator variables are unambiguous.
♻️ Duplicate comments (5)
src/bun_alloc/ast_alloc.rs (1)
192-203:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDon't preserve the bump cursor across
set_thread_heap(null).
bump_invalidate_heap()only clears the TLS of the thread doing the destroy/reset. BecauseMimallocArenaisSend, a heap can still be reset or dropped on thread B while thread A keeps its oldBUMP_HEAP/cursor alive after this null install. If mimalloc later reuses that heap address, this equality fast path can resurrect freed bump storage.Safe fallback
pub fn set_thread_heap(heap: *mut mimalloc::Heap) { AST_HEAP.set(heap); - if !heap.is_null() && heap != BUMP_HEAP.get() { + if heap.is_null() || heap != BUMP_HEAP.get() { bump_reset(); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bun_alloc/ast_alloc.rs` around lines 192 - 203, The bump cursor must not be preserved when installing a null heap; modify set_thread_heap to call bump_reset() not only when switching to a different non-null heap but also when heap.is_null() so the TLS cursor is cleared on null install. Update the condition around bump_reset() in set_thread_heap (which reads AST_HEAP/BUMP_HEAP and calls bump_reset) to trigger reset when heap.is_null() || heap != BUMP_HEAP.get(), ensuring no stale cursor survives a null install while leaving other logic intact (leave bump_invalidate_heap and MimallocArena semantics unchanged).src/bundler/LinkerGraph.rs (1)
629-633:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winStill skip non-AST sources before rewriting SCB import records.
reachable_filescan still contain CSS/HTML/assets, butitems_import_records_mut()only has rows for AST-backed sources. Line 632 indexes it unconditionally, so this SCB rewrite can still panic on a reachable non-JS file.💡 Minimal fix
let import_records_list: &mut [import_record::List<'_>] = self.ast.items_import_records_mut(); for source_id in self.reachable_files.slice() { - for import_record in import_records_list[source_id.get() as usize] + let source_index = source_id.get() as usize; + if source_index >= import_records_list.len() { + continue; + } + + for import_record in import_records_list[source_index] .as_mut_slice() .iter_mut() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bundler/LinkerGraph.rs` around lines 629 - 633, The loop that rewrites SCB import records indexes into import_records_list using entries from reachable_files without ensuring the source is AST-backed; update the loop in LinkerGraph so you skip non-AST sources before indexing into import_records_list (e.g., check whether the source_id refers to an AST-backed source or is within the bounds/has an entry in items_import_records_mut) to avoid panics when reachable_files contains CSS/HTML/assets; specifically guard the iteration over self.reachable_files.slice() (the for source_id ...) and only access import_records_list[source_id.get() as usize] when that source is known to have AST import records (or use a safe get and continue when None).src/bundler/ThreadPool.rs (1)
637-680:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winInvalidate
TLS_WORKERbefore freeing the worker.
get_worker()only keys the fast path onself.generation.deinit()frees the boxedWorkerwithout clearing the current thread's cached pointer, so a later lookup on the same pool can return a dangling worker and skip the guarded map entirely.🛠️ Minimal fix
pub unsafe fn deinit(this: *mut Worker) { + if TLS_WORKER.get().1 == this { + TLS_WORKER.set((0, core::ptr::null_mut())); + } + // SAFETY: caller contract. let worker = unsafe { &mut *this };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bundler/ThreadPool.rs` around lines 637 - 680, The deinit function currently frees the Worker without clearing the thread-local cache, leaving TLS_WORKER pointing at freed memory; update deinit (the unsafe fn deinit(this: *mut Worker)) to invalidate/clear TLS_WORKER (the thread-local cached pointer used by get_worker()) early — before dropping fields, arenas, or calling bun_core::heap::destroy(this) — so any subsequent get_worker() fast-path sees no valid cached worker and falls back to the guarded lookup; ensure you perform the TLS clear in both branches where worker.has_created is true or false and before any operation that frees or destroys the Worker.src/js_parser/parse/parse_entry.rs (1)
318-320:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRestore the
'alifetime onloginParser::init.Line 320 now lets
Parser<'a>retain aNonNull<bun_ast::Log>derived from a shorter-lived&mut bun_ast::Log.log_mut()later dereferences that raw pointer, so this drops the only type-level guarantee that the logger outlives the parser.Suggested fix
pub fn init( options: Options<'a>, - log: &mut bun_ast::Log, + log: &'a mut bun_ast::Log, source: &'a bun_ast::Source, define: &'a Define, bump: &'a Arena,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/js_parser/parse/parse_entry.rs` around lines 318 - 320, The Parser::init signature must accept the logger with the parser lifetime so the NonNull<bun_ast::Log> you store remains valid; change the parameter from log: &mut bun_ast::Log to log: &'a mut bun_ast::Log in Parser::init (and propagate that 'a where needed) so the Parser<'a> retains a pointer derived from a same‑lifetime reference; ensure any places constructing NonNull::new_unchecked or calling log_mut() still use that &'a mut bun_ast::Log to preserve the type‑level lifetime guarantee.src/resolver/resolver.rs (1)
3221-3227:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
styleconditions in the global-cache exports path.These two
ESModuleinitializers still routeImportKind::At/ImportKind::AtConditionaltoself.opts.conditions.import, while the localnode_modulesbranch above usesself.opts.conditions.style. That makes CSS package imports resolve against differentexportsentries depending on whether the package came from a local install or the auto-install/global cache.Suggested fix
let esm_resolution = ESModule { conditions: match kind { ast::ImportKind::Require | ast::ImportKind::RequireResolve => { &self.opts.conditions.require } + ast::ImportKind::At + | ast::ImportKind::AtConditional => { + &self.opts.conditions.style + } _ => &self.opts.conditions.import, }, debug_logs: self.debug_logs.as_mut(), module_type: &mut module_type, } @@ let esm_resolution = ESModule { conditions: match kind { ast::ImportKind::Require | ast::ImportKind::RequireResolve => { &self.opts.conditions.require } + ast::ImportKind::At + | ast::ImportKind::AtConditional => { + &self.opts.conditions.style + } _ => &self.opts.conditions.import, }, debug_logs: self.debug_logs.as_mut(), module_type: &mut module_type, }Also applies to: 3257-3263
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/resolver/resolver.rs` around lines 3221 - 3227, The ESModule initializer in the global-cache exports path currently maps ast::ImportKind::At and AtConditional to self.opts.conditions.import causing CSS/style imports to resolve differently; update the match that sets conditions (the branch handling ast::ImportKind::Require | RequireResolve vs others) so that ImportKind::At and ImportKind::AtConditional use self.opts.conditions.style instead of self.opts.conditions.import; apply the same change to the analogous match further down (the other ESModule initializer referenced in the comment) so both global-cache paths use self.opts.conditions.style for style imports.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ast/nodes.rs`:
- Line 1140: PartList<'a> now places Part instances in the arena but Part still
owns drop-managed containers (Part.dependencies, Part.symbol_uses,
Part.import_symbol_property_uses) whose destructors won't run when the Mimalloc
arena resets, leaking per-file state; fix this by migrating those fields to
arena-backed containers or freeing them explicitly before the arena reset:
replace Vec/ArrayHashMap usages inside struct Part with arena-allocated
equivalents (e.g., bun_alloc::ArenaVec<'a, T> or your AstAlloc-backed map types)
or convert them to indices/refs into arena-owned storage, and if migration isn't
possible ensure code paths that clear/free Part::dependencies, Part::symbol_uses
and Part::import_symbol_property_uses are called before the
bun_alloc::MimallocArena reset so no Drop-dependent resources remain.
In `@src/bun_alloc/baby_vec.rs`:
- Around line 141-145: The reserve()/push overflow bug: replace unchecked
addition and clamping with a hard check using checked_add and a capacity ceiling
check against u32::MAX (or MAX as used by the vec implementation) and fail early
instead of clamping; specifically modify reserve (and similar spots at the other
ranges) to compute let need = self.len.checked_add(additional).and_then(|n| if n
<= MAX { Some(n) } else { None }) and abort/panic (or return an Err) when None,
and change grow_exact/grow_to to propagate that failure rather than silently
clamping — ensure push uses the same checked logic so it cannot proceed when len
== cap.
- Around line 303-315: The current drain<R: RangeBounds<usize>> implementation
only uses debug_assert! so partial ranges silently drain everything in release;
either enforce the precondition at runtime or narrow the API to a full-drain
method. Fix option A: replace the debug_assert! checks in pub fn drain<R:
RangeBounds<usize>>(&mut self, range: R) with real runtime validation (inspect
range.start_bound() and range.end_bound(), and if they don’t match
Unbounded/Included(0) and the expected end matching self.len, call panic! with a
clear message referencing the provided range) before doing
core::mem::replace(self, BabyVec::new_in(self.alloc)).into_iter(). Fix option B:
remove the RangeBounds generic and change the signature to pub fn drain(&mut
self) -> IntoIter<'a, T> (and drop the range parameter and its checks), making
it an explicit full-drain operation; update callers accordingly.
In `@src/bun_alloc/lib.rs`:
- Around line 291-310: transfer_arena currently only calls v.set_allocator(dst)
which leaves the existing buffer owned by the source arena; instead perform an
eager migration of the backing buffer into dst: inside transfer_arena, allocate
a new buffer on dst (using MimallocArena APIs / mi_heap_realloc_aligned or
mi_heap_malloc with the current capacity/usable size), memcpy the existing bytes
from v's pointer into the new buffer, free the old buffer (mi_free) from the
source arena, then update ArenaVec's internal pointer/length/capacity and call
v.set_allocator(dst) so the Vec truly lives on dst; alternatively, if you prefer
not to move memory, replace transfer_arena with a narrower API that documents
and enforces the source MimallocArena outliving the ArenaVec (e.g., rename to
reassign_allocator_unchecked and require caller responsibility).
In `@src/bun_core/thread_id.rs`:
- Around line 124-125: Replace the nightly-only attribute usage by changing the
bare #[thread_local] static TLS_THREAD_ID (type ThreadId) to use the stable
thread_local! macro: declare TLS_THREAD_ID via thread_local! with a const
initializer that constructs a core::cell::Cell<ThreadId> initialized to 0, so
accesses to TLS_THREAD_ID remain fast and avoid the LocalKey state-machine;
update any direct static references to use TLS_THREAD_ID.with(...) or the
appropriate LocalKey API where the code previously read/wrote the static Cell.
In `@src/bundler/AstBuilder.rs`:
- Around line 450-458: The HMR temp symbol created by
generate_temp_ref(temp_export) is recorded in declared_symbols/current_scope but
not added to top_level_symbols_to_parts and is then redundantly appended into
current_scope.generated; instead, after creating temp_id and updating
parts[1].declared_symbols and symbol_uses, register temp_id in
top_level_symbols_to_parts mapped to the part (parts[1] / part index 1) so the
BundledAst has the part mapping for that top-level symbol, and remove the
redundant VecExt::append(&mut self.current_scope_mut().generated, temp_id) call.
In `@src/bundler/bundle_v2.rs`:
- Around line 7272-7275: The replacement AST is being allocated on the parse
worker's thread-local heap via result.ast.parts.allocator() (result_heap), which
can cross threads and segfault; in on_parse_task_complete replace the moved-from
placeholder with an allocation that uses the bundle thread's heap (e.g., use
self.graph.heap) or an allocation-free tombstone instead of
JSAst::empty_in(result_heap), and keep the move into this.graph.ast at
result_source_index using core::mem::replace(&mut result.ast, /* safe
placeholder that does not use result_heap */) so no allocation happens on the
worker arena.
In `@src/bundler/ThreadPool.rs`:
- Around line 558-567: The arena() method must not return a safe 'static
reference because Worker::deinit() frees the heap (bun_core::heap::take()),
making cached &'static references unsound; change pub fn arena(&self) ->
&'static ThreadLocalArena to return a reference tied to &self (e.g. pub fn
arena(&self) -> &ThreadLocalArena) and remove the use of
bun_ptr::detach_lifetime_ref; instead obtain a temporary reference from the
BackRef (self.arena.get() or equivalent) and cast it to a reference with the
same lifetime as &self (or keep it unsafe inside a private unsafe helper that
callers must not store), update callers of ThreadPool::arena / ThreadLocalArena
to accept a non-'static borrow, and ensure Worker::deinit and any heap-take code
cannot be called while such borrows are live.
In `@src/js_parser/p.rs`:
- Line 8361: The parser currently moves module_scope into the returned Ast but
leaves ts_namespace_scopes and ts_namespace_member_maps owned by P, so when
P::drop() runs those maps are freed and any Ast scopes holding ts_namespace
handles become dangling; fix by transferring these TS-namespace sidecars into
the Ast during to_ast()/to_ast-like return (or, if the AST must not own them,
clear all scope references to ts_namespace handles before P is dropped).
Concretely, update P::to_ast()/to_module_ast (the code paths that move
module_scope into js_ast::Ast<'a>) to also move ts_namespace_scopes and
ts_namespace_member_maps into the returned Ast struct (or set the corresponding
scope fields to None/clear handles) and remove freeing of those maps from
P::drop() so the Ast owns the sidecars and no dangling references remain.
---
Outside diff comments:
In `@src/ast/ast_result.rs`:
- Around line 23-61: The Ast<'a> struct still contains heap-owned fields
(export_star_import_records, top_level_symbols_to_parts including its inner
Vec<u32>s, and ts_enums with their StringHashMaps) that rely on Drop and
therefore leak when the arena is reset; switch those fields to arena-backed
collections (e.g., replace Vec/HashMap/StringHashMap with the project's arena
equivalents or Store/PartList-style types used elsewhere) or add an explicit
teardown method (e.g., Ast::free_arena_owned_fields or Ast::teardown) that
manually clears/frees export_star_import_records, top_level_symbols_to_parts
(iterate and clear inner Vec<u32>s), and ts_enums maps before the arena reset;
update constructors and any code that mutates these fields to allocate into the
arena variants and ensure the new teardown is called where the arena is reset.
In `@src/bundler/LinkerContext.rs`:
- Around line 1898-1910: The loop over import_records calls validate_tla with
ast_import_records[record.source_index.get() as usize].as_slice() without
re-checking that record.source_index points to an AST-bearing entry; to fix, add
a guard before indexing (e.g. ensure record.source_index.get() as usize <
ast_import_records.len() or compare against import_records_len) and skip or
handle non-AST targets so validate_tla is only invoked with a valid slice;
update the code paths around the for loop and the validate_tla(...) call to use
a checked access (or early continue) when the source_index is out of bounds.
In `@src/bundler/transpiler.rs`:
- Around line 1779-1848: The AlreadyBundled branch of js_ast::Result constructs
a ParseResult but currently sets runtime_transpiler_cache to None, dropping the
previously computed rtc_ptr; update the ParseResult construction (in the
js_ast::Result::AlreadyBundled arm) to forward the computed rtc_ptr into
runtime_transpiler_cache (use the same value/name used earlier, e.g., rtc_ptr or
the variable that holds the runtime transpiler cache) so cache propagation is
preserved for the AlreadyBundled fast path; ensure the field type matches
(wrap/clone/transfer as needed) when assigning to runtime_transpiler_cache in
the ParseResult returned alongside ast, already_bundled, source, loader,
input_fd, etc.
In `@src/js_parser/lexer.rs`:
- Around line 2686-2705: The public constructors (init, init_json,
init_without_reading) accept &mut Log but the struct stores NonNull<Log> and
log() reconstitutes &mut Log, creating a potential use-after-free; restore the
borrow by changing the constructors to accept &'a mut Log (or add an explicit
'_log lifetime on the struct) so the compiler enforces that the Log outlives the
lexer, or else mark these constructors unsafe/internal and document the safety
requirements; update the signatures of init, init_json, and init_without_reading
and any callers accordingly, and adjust the field/lifetime comments to match the
chosen approach.
In `@src/runtime/api/JSTranspiler.rs`:
- Around line 801-825: get_parse_result() currently lies by returning
ParseResult<'static> while on the non-REPL path processed_code is an alias to
the caller's code buffer, so the AST borrows non-'static data; fix by either (A)
copying non-REPL bytes into the allocated arena before creating the
arena_ref/source so the AST truly borrows arena-owned data (update the non-REPL
branch where processed_code is set and ensure arena_ref.alloc is used for the
bytes), or (B) change the function signature and ParseResult usage to be
lifetime-generic (e.g., ParseResult<'a>) so the return reflects the caller-owned
lifetime and update callers of get_parse_result() accordingly (adjust types at
call sites that consume ParseResult). Ensure modifications reference
get_parse_result, ParseResult<'static>, processed_code, source, and
arena/arena_ref so the borrowed lifetimes are consistent.
In `@src/runtime/cli/repl.rs`:
- Around line 1852-1866: The code in transform_for_repl rebinds the variable
arena (first used as the parse arena) when assigning from
ast.symbols.allocator(), which shadows the original and reduces clarity; rename
the original parse arena binding to parse_arena and rename the allocator binding
to symbols_alloc (or similar) wherever they are declared and used (e.g., the
initial parse arena declaration around line ~1783 and the later let arena =
*ast.symbols.allocator(); used by print_ast and bun_alloc::ArenaVec::new_in) and
update all references in this function (including calls to
bun_js_printer::print_ast, bun_alloc::ArenaVec::new_in, and symbol Map
initialization) so the two distinct allocator variables are unambiguous.
---
Duplicate comments:
In `@src/bun_alloc/ast_alloc.rs`:
- Around line 192-203: The bump cursor must not be preserved when installing a
null heap; modify set_thread_heap to call bump_reset() not only when switching
to a different non-null heap but also when heap.is_null() so the TLS cursor is
cleared on null install. Update the condition around bump_reset() in
set_thread_heap (which reads AST_HEAP/BUMP_HEAP and calls bump_reset) to trigger
reset when heap.is_null() || heap != BUMP_HEAP.get(), ensuring no stale cursor
survives a null install while leaving other logic intact (leave
bump_invalidate_heap and MimallocArena semantics unchanged).
In `@src/bundler/LinkerGraph.rs`:
- Around line 629-633: The loop that rewrites SCB import records indexes into
import_records_list using entries from reachable_files without ensuring the
source is AST-backed; update the loop in LinkerGraph so you skip non-AST sources
before indexing into import_records_list (e.g., check whether the source_id
refers to an AST-backed source or is within the bounds/has an entry in
items_import_records_mut) to avoid panics when reachable_files contains
CSS/HTML/assets; specifically guard the iteration over
self.reachable_files.slice() (the for source_id ...) and only access
import_records_list[source_id.get() as usize] when that source is known to have
AST import records (or use a safe get and continue when None).
In `@src/bundler/ThreadPool.rs`:
- Around line 637-680: The deinit function currently frees the Worker without
clearing the thread-local cache, leaving TLS_WORKER pointing at freed memory;
update deinit (the unsafe fn deinit(this: *mut Worker)) to invalidate/clear
TLS_WORKER (the thread-local cached pointer used by get_worker()) early — before
dropping fields, arenas, or calling bun_core::heap::destroy(this) — so any
subsequent get_worker() fast-path sees no valid cached worker and falls back to
the guarded lookup; ensure you perform the TLS clear in both branches where
worker.has_created is true or false and before any operation that frees or
destroys the Worker.
In `@src/js_parser/parse/parse_entry.rs`:
- Around line 318-320: The Parser::init signature must accept the logger with
the parser lifetime so the NonNull<bun_ast::Log> you store remains valid; change
the parameter from log: &mut bun_ast::Log to log: &'a mut bun_ast::Log in
Parser::init (and propagate that 'a where needed) so the Parser<'a> retains a
pointer derived from a same‑lifetime reference; ensure any places constructing
NonNull::new_unchecked or calling log_mut() still use that &'a mut bun_ast::Log
to preserve the type‑level lifetime guarantee.
In `@src/resolver/resolver.rs`:
- Around line 3221-3227: The ESModule initializer in the global-cache exports
path currently maps ast::ImportKind::At and AtConditional to
self.opts.conditions.import causing CSS/style imports to resolve differently;
update the match that sets conditions (the branch handling
ast::ImportKind::Require | RequireResolve vs others) so that ImportKind::At and
ImportKind::AtConditional use self.opts.conditions.style instead of
self.opts.conditions.import; apply the same change to the analogous match
further down (the other ESModule initializer referenced in the comment) so both
global-cache paths use self.opts.conditions.style for style imports.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5f6d08a9-adaf-46cf-839c-98fc297ebbd0
📒 Files selected for processing (74)
src/ast/ast_result.rssrc/ast/import_record.rssrc/ast/nodes.rssrc/ast/symbol.rssrc/bun_alloc/MimallocArena.rssrc/bun_alloc/ast_alloc.rssrc/bun_alloc/baby_vec.rssrc/bun_alloc/lib.rssrc/bun_core/lib.rssrc/bun_core/thread_id.rssrc/bun_core/util.rssrc/bundler/AstBuilder.rssrc/bundler/Chunk.rssrc/bundler/Graph.rssrc/bundler/HTMLImportManifest.rssrc/bundler/LinkerContext.rssrc/bundler/LinkerGraph.rssrc/bundler/ParseTask.rssrc/bundler/ServerComponentParseTask.rssrc/bundler/ThreadPool.rssrc/bundler/barrel_imports.rssrc/bundler/bundle_v2.rssrc/bundler/bundled_ast.rssrc/bundler/cache.rssrc/bundler/lib.rssrc/bundler/linker.rssrc/bundler/linker_context/MetafileBuilder.rssrc/bundler/linker_context/StaticRouteVisitor.rssrc/bundler/linker_context/computeCrossChunkDependencies.rssrc/bundler/linker_context/convertStmtsForChunk.rssrc/bundler/linker_context/convertStmtsForChunkForDevServer.rssrc/bundler/linker_context/doStep5.rssrc/bundler/linker_context/findAllImportedPartsInJSOrder.rssrc/bundler/linker_context/findImportedCSSFilesInJSOrder.rssrc/bundler/linker_context/findImportedFilesInCSSOrder.rssrc/bundler/linker_context/generateCodeForFileInChunkJS.rssrc/bundler/linker_context/generateCodeForLazyExport.rssrc/bundler/linker_context/generateCompileResultForHtmlChunk.rssrc/bundler/linker_context/postProcessJSChunk.rssrc/bundler/linker_context/prepareCssAstsForChunk.rssrc/bundler/linker_context/renameSymbolsInChunk.rssrc/bundler/linker_context/scanImportsAndExports.rssrc/bundler/transpiler.rssrc/bundler/ungate_support.rssrc/css/css_parser.rssrc/css/dependencies.rssrc/css/printer.rssrc/css/properties/custom.rssrc/css/selectors/selector.rssrc/css/values/ident.rssrc/css/values/url.rssrc/js_parser/lexer.rssrc/js_parser/lib.rssrc/js_parser/p.rssrc/js_parser/parse/parse_entry.rssrc/js_parser/parser.rssrc/js_printer/lib.rssrc/jsc/AsyncModule.rssrc/jsc/RuntimeTranspilerStore.rssrc/jsc/lib.rssrc/paths/resolve_path.rssrc/resolver/lib.rssrc/resolver/package_json.rssrc/resolver/resolver.rssrc/runtime/api/JSTranspiler.rssrc/runtime/api/js_bundle_completion_task.rssrc/runtime/bake/DevServer.rssrc/runtime/bake/dev_server/incremental_graph.rssrc/runtime/cli/create/SourceFileProjectGenerator.rssrc/runtime/cli/repl.rssrc/runtime/cli/test/ChangedFilesFilter.rssrc/runtime/jsc_hooks.rssrc/runtime/test_runner/snapshot.rssrc/threading/ThreadPool.rs
Ungated fetch_add(Relaxed) on shared counters in the notify() hot path, accidentally swept into e21d0b8 from an abandoned investigation.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/bun_alloc/baby_vec.rs (1)
141-146:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnchecked addition in
reservecan wrap, bypassing the capacity check.If
additionalis large (close tousize::MAX), the additionself.len as usize + additionalcan wrap around to a small value. This would causeneed > self.capto be false, skipping the grow, butpushwould later succeed because the capacity check there uses a different path. Theassert!ingrow_toonly fires if the grow path is taken.The past review flagged this same issue. Consider using
checked_addor saturating arithmetic:Suggested fix
pub fn reserve(&mut self, additional: usize) { - let need = self.len as usize + additional; + let need = (self.len as usize) + .checked_add(additional) + .expect("BabyVec capacity overflow"); if need > self.cap as usize { self.grow_to(need); } }Also applies to
reserve_exactat lines 149-154.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bun_alloc/baby_vec.rs` around lines 141 - 146, The addition in reserve (and similarly in reserve_exact) can overflow and wrap, bypassing the capacity check; change the calculation of need in reserve and reserve_exact to use checked or saturating arithmetic (e.g., use self.len.checked_add(additional).unwrap_or(usize::MAX) or self.len.saturating_add(additional)) before comparing to self.cap, then call grow_to(need) if need > self.cap; reference the reserve, reserve_exact functions and grow_to so you update both methods to compute a non-wrapping need value and handle the saturated/overflow case consistently.src/bundler/ThreadPool.rs (2)
423-435:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winTLS_WORKER cache not invalidated when Worker is freed — use-after-free risk.
The fast path returns the cached
Workerpointer whengenerationmatches, butWorker::deinit()frees theWorkerwithout clearingTLS_WORKER. If the same thread later callsget_worker()on the same pool (same generation), it will return a dangling pointer.The past review suggested clearing the TLS entry in
Worker::deinit:Minimal fix
pub unsafe fn deinit(this: *mut Worker) { + let (gen, cached) = TLS_WORKER.get(); + if cached == this { + TLS_WORKER.set((0, core::ptr::null_mut())); + } + // SAFETY: caller contract. let worker = unsafe { &mut *this };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bundler/ThreadPool.rs` around lines 423 - 435, The TLS_WORKER cached pointer can become dangling because Worker::deinit frees the Worker without clearing the thread-local cache; update Worker::deinit to invalidate the TLS entry (TLS_WORKER) for the current thread so get_worker()’s fast path (which checks generation against ThreadPool::generation) cannot return a freed pointer — ensure the TLS slot is cleared/zeroed before freeing or setting generation to a non-matching value so subsequent calls to get_worker() fall through to get_worker_slow(id).
558-570:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift
arena()returning&'staticis unsound — Worker can be freed while references exist.The safety comment claims "callers are task callbacks that complete before
deinit_soon", but the returned&'staticreference can be stored and used after the Worker is freed viadeinit(). This allows safe Rust code to access freed memory.Consider either:
- Return
&ThreadLocalArena(tied to&selflifetime), or- Keep it
unsafe fn arena_static(&self) -> &'static ThreadLocalArenawith explicit safety requirementsSuggested fix
- pub fn arena(&self) -> &'static ThreadLocalArena { - // SAFETY: ... - unsafe { bun_ptr::detach_lifetime_ref(self.arena.get()) } - } + pub fn arena(&self) -> &ThreadLocalArena { + self.arena.get() + } + + /// # Safety + /// The returned reference must not outlive this worker. + pub unsafe fn arena_static(&self) -> &'static ThreadLocalArena { + unsafe { bun_ptr::detach_lifetime_ref(self.arena.get()) } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bundler/ThreadPool.rs` around lines 558 - 570, The arena() method is unsound because it returns a &'static ThreadLocalArena that can outlive the Worker and be used after deinit()/deinit_soon; change the API to either (A) make arena(&self) -> &ThreadLocalArena (remove the 'static and tie the lifetime to &self) and stop using bun_ptr::detach_lifetime_ref, or (B) rename to unsafe fn arena_static(&self) -> &'static ThreadLocalArena and document explicit safety requirements (caller must guarantee the Worker outlives any reference, reference must not be stored across deinit()/deinit_soon), updating call sites and Worker::create / Worker::get expectations accordingly; reference symbols: arena(), ThreadLocalArena, bun_ptr::detach_lifetime_ref, BackRef, Worker::create, Worker::get, deinit_soon, deinit().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/bun_alloc/baby_vec.rs`:
- Around line 141-146: The addition in reserve (and similarly in reserve_exact)
can overflow and wrap, bypassing the capacity check; change the calculation of
need in reserve and reserve_exact to use checked or saturating arithmetic (e.g.,
use self.len.checked_add(additional).unwrap_or(usize::MAX) or
self.len.saturating_add(additional)) before comparing to self.cap, then call
grow_to(need) if need > self.cap; reference the reserve, reserve_exact functions
and grow_to so you update both methods to compute a non-wrapping need value and
handle the saturated/overflow case consistently.
In `@src/bundler/ThreadPool.rs`:
- Around line 423-435: The TLS_WORKER cached pointer can become dangling because
Worker::deinit frees the Worker without clearing the thread-local cache; update
Worker::deinit to invalidate the TLS entry (TLS_WORKER) for the current thread
so get_worker()’s fast path (which checks generation against
ThreadPool::generation) cannot return a freed pointer — ensure the TLS slot is
cleared/zeroed before freeing or setting generation to a non-matching value so
subsequent calls to get_worker() fall through to get_worker_slow(id).
- Around line 558-570: The arena() method is unsound because it returns a
&'static ThreadLocalArena that can outlive the Worker and be used after
deinit()/deinit_soon; change the API to either (A) make arena(&self) ->
&ThreadLocalArena (remove the 'static and tie the lifetime to &self) and stop
using bun_ptr::detach_lifetime_ref, or (B) rename to unsafe fn
arena_static(&self) -> &'static ThreadLocalArena and document explicit safety
requirements (caller must guarantee the Worker outlives any reference, reference
must not be stored across deinit()/deinit_soon), updating call sites and
Worker::create / Worker::get expectations accordingly; reference symbols:
arena(), ThreadLocalArena, bun_ptr::detach_lifetime_ref, BackRef,
Worker::create, Worker::get, deinit_soon, deinit().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3c269d77-bb3d-41cd-9bc9-401093b386f1
📒 Files selected for processing (5)
src/ast/ast_result.rssrc/bun_alloc/baby_vec.rssrc/bundler/ThreadPool.rssrc/bundler/ungate_support.rssrc/jsc/lib.rs
| /// can observe the `Worker`, and is never dangling after that point. The | ||
| /// pointee is the worker's own `heap` field, which is pinned for the | ||
| /// worker's lifetime. | ||
| /// The worker-owned bump arena. Returns `&'static` because the arena is | ||
| /// pinned for the worker's lifetime and `Worker::get` already hands out a | ||
| /// `&'static mut Worker`; centralising the erasure here avoids per-call-site | ||
| /// `detach_lifetime_ref` (the previous pattern at `ParseTask::run`). |
There was a problem hiding this comment.
🟡 The new doc comment for Worker::arena() (lines 558-561) was appended without removing the old one (lines 550-557), so rustdoc renders a single run-on paragraph: "...pinned for the worker's lifetime. The worker-owned bump arena. Returns &'static...". The old paragraph ("Reborrow the self-referential arena... BackRef::get... see PORT NOTE on the field") describes the previous tied-borrow implementation and is now stale/misleading next to the new 'static-erasure description. Delete lines 550-557 so only the new doc remains.
Extended reasoning...
What the issue is
The diff for Worker::arena() at src/bundler/ThreadPool.rs:550-568 shows the new doc comment (lines 558-561) was appended after the old one (lines 550-557) rather than replacing it. The diff hunk only contains + lines for the new doc; the old lines appear as unchanged context:
/// Reborrow the self-referential `arena` (= `&self.heap`) as a shared
/// reference. `BackRef` field, so the deref is encapsulated in
/// [`bun_ptr::BackRef::get`]; see PORT NOTE on the field.
///
/// `arena` is set to `&self.heap` in [`Worker::create`] before any caller
/// can observe the `Worker`, and is never dangling after that point. The
/// pointee is the worker's own `heap` field, which is pinned for the
/// worker's lifetime.
/// The worker-owned bump arena. Returns `&'static` because the arena is
/// pinned for the worker's lifetime and `Worker::get` already hands out a
/// `&'static mut Worker`; centralising the erasure here avoids per-call-site
/// `detach_lifetime_ref` (the previous pattern at `ParseTask::run`).
How it manifests
Rustdoc concatenates contiguous /// lines into a single doc attribute. Because line 557 ("...pinned for the worker's lifetime.") and line 558 ("The worker-owned bump arena. Returns &'static...") are adjacent with no blank /// separator, they render as a single run-on paragraph in the generated docs:
...The pointee is the worker's own
heapfield, which is pinned for the worker's lifetime. The worker-owned bump arena. Returns&'staticbecause...
Beyond the run-on, the content of the old paragraph is now wrong. It says "Reborrow the self-referential arena (= &self.heap) as a shared reference" and points readers at BackRef::get — that described the previous signature (pub fn arena(&self) -> &ThreadLocalArena { self.arena.get() }), which returned a borrow tied to &self. The new implementation wraps the result in unsafe { bun_ptr::detach_lifetime_ref(...) } and returns &'static, which is exactly what the new paragraph (lines 558-561) documents. The two paragraphs describe two different APIs.
Step-by-step proof
- Before this PR, lines 550-557 documented
pub fn arena(&self) -> &ThreadLocalArena. - The PR changes the signature to
-> &'static ThreadLocalArenaand the body tounsafe { detach_lifetime_ref(...) }. - The author wrote a replacement doc comment (lines 558-561) explaining the new
'staticreturn. - The diff shows only
+lines for 558-561; lines 550-557 are context (no-), so they were never deleted. - Result: 12 contiguous
///lines forming one doc block, with the second-to-last sentence of the old paragraph (line 557) glued directly onto the first sentence of the new paragraph (line 558).
Why nothing prevents it
This is pure documentation — the compiler doesn't validate doc-comment content, and rustdoc happily concatenates the lines. It's an editing oversight: the author wrote a replacement but forgot to delete the original.
Not a duplicate
This is distinct from the two existing PR comments on this function:
- claude[bot] (id 3263968658, line 568) is about the SAFETY block inside the function body claiming "process lifetime" — that's lines 564-566, not the rustdoc above the signature.
- coderabbit (id 3264535565, line 567) is about the soundness of returning
&'staticfrom a safe fn — also about the implementation, not the duplicated doc.
Neither comment mentions the concatenated/stale doc block at 550-561.
Impact
Doc-only — no runtime effect. The rendered rustdoc for Worker::arena() reads as a confusing run-on that first describes the old behavior (tied borrow via BackRef::get) and then contradicts itself by describing the new behavior ('static erasure). A reader following the first half would form the wrong mental model.
Fix
Delete lines 550-557 so the doc comment is just the new paragraph:
/// The worker-owned bump arena. Returns `&'static` because the arena is
/// pinned for the worker's lifetime and `Worker::get` already hands out a
/// `&'static mut Worker`; centralising the erasure here avoids per-call-site
/// `detach_lifetime_ref` (the previous pattern at `ParseTask::run`).
#[inline]
pub fn arena(&self) -> &'static ThreadLocalArena {| /// borrows. | ||
| #[inline] | ||
| pub fn parse_graph_mut(&mut self) -> &mut Graph { | ||
| pub fn parse_graph_mut(&mut self) -> &mut Graph<'a> { |
There was a problem hiding this comment.
🔴 Now that Graph is Graph<'a>, parse_graph() here still returns bare &Graph while the sibling parse_graph_mut() was updated to -> &mut Graph<'a> — the mixed elision triggers rustc's mismatched_lifetime_syntaxes lint, which robobun surfaces for this line in Build #56028. The same bare &Graph (next to a PR-updated &LinkerGraph<'_> in the same signature) is also left at Chunk.rs:545/596/641, HTMLImportManifest.rs:136/180, and ungate_support.rs:549; change this one to -> &Graph<'a> and the others to &Graph<'_>.
Extended reasoning...
What the issue is
This PR makes Graph lifetime-generic (Graph<'a>) and updates LinkerContext::parse_graph_mut() at line 309 to return &mut Graph<'a>, but leaves the immediately-adjacent shared accessor parse_graph() at line 294 returning bare &Graph (path-elided lifetime). Mixing an explicit lifetime argument on one accessor with full path-elision on its sibling triggers rustc's mismatched_lifetime_syntaxes lint ("hiding a lifetime that's elided elsewhere is confusing"), which CI surfaces in the robobun report for Build #56028 (🍎 x64 - build-rust: src/bundler/LinkerContext.rs:294).
The same incomplete update appears at six other parameter sites where this PR changed the adjacent &LinkerGraph parameter to &LinkerGraph<'_> but left the &Graph parameter in the same signature bare:
src/bundler/Chunk.rs:545,596,641—parse_graph: &Graph,/graph: &Graph,src/bundler/HTMLImportManifest.rs:136,180—graph: &Graph,src/bundler/ungate_support.rs:549—graph: &Graph,
Why nothing prevents it
This compiles: &Graph with a path-elided lifetime is still valid Rust, and mismatched_lifetime_syntaxes is warn-by-default (I checked Cargo.toml workspace lints, the bundler crate's inner attrs, and scripts/build/rust.ts rustflags — none promote it to deny). The robobun comment lists it alongside genuine job failures and pre-existing deprecation warnings, so it appears to be surfaced as an informational diagnostic rather than a hard error. That said, it's a new diagnostic introduced by this PR on lines the PR directly touched, and it's an obvious half-finished refactor: every one of these signatures had its LinkerGraph parameter updated by this PR while the Graph parameter one line above was skipped.
Step-by-step proof
- Before this PR,
Graphhad no lifetime parameter, so-> &Graphat line 294 and-> &mut Graphat line 309 were symmetric and lint-clean. - This PR changes
pub struct Graph→pub struct Graph<'a>(Graph.rs:20) and updates line 309 to-> &mut Graph<'a>, but the diff has no hunk for line 294. - At current HEAD (3f4f1c3, after the PR's fixup commits), line 294 still reads
pub fn parse_graph(&self) -> &Graph {and line 309 reads-> &mut Graph<'a>. - rustc sees one method on
impl<'a> LinkerContext<'a>spelling the type asGraph<'a>and the other as bareGraph, and emitsmismatched_lifetime_syntaxeson the elided one. - robobun's Build #56028 report (most recent on this PR) lists exactly this:
src/bundler/LinkerContext.rs:294 - hiding a lifetime that's elided elsewhere is confusing. git log 1d1f2ed..HEADshows none of the five subsequent commits touch line 294 or any of the six other listed sites.
For the parameter sites: the diff hunks at Chunk.rs:543-547 / 594-598 / 639-643, HTMLImportManifest.rs:134-138 / 178-182, and ungate_support.rs:547-551 each show the PR changing linker_graph: &LinkerGraph, → linker_graph: &LinkerGraph<'_>, while leaving the graph: &Graph, line directly above as untouched context.
Relationship to existing PR comments
Not a duplicate. CodeRabbit comment #3263939941 covered only HTMLImportManifest.rs (and is marked "addressed", though grep confirms the bare &Graph at lines 136/180 remains — that comment's actual fix was the from_ref::<Graph> turbofish, not the function signatures). The robobun comment is a raw CI diagnostic listing without explanation. No existing comment covers LinkerContext.rs:294 (the line CI actually flags), Chunk.rs, or ungate_support.rs.
Impact
A new compiler lint diagnostic on every build, surfaced in CI, on code this PR directly modified. Whether it hard-fails the build depends on whether the build-rust job treats new warnings as failures (the diagnostic list mixes pre-existing items, so likely not), but either way it's an inconsistency the PR introduced and should clean up before merge — the PR already established the Graph<'a> / <'_> convention everywhere else it touched these types.
Fix
LinkerContext.rs:294:-> &Graph→-> &Graph<'a>(matchingparse_graph_mutat 309)Chunk.rs:545,596,641,HTMLImportManifest.rs:136,180,ungate_support.rs:549:&Graph,→&Graph<'_>,(matching the adjacent&LinkerGraph<'_>the PR already updated)
#31069) Fixes flaky `test/regression/issue/03830.test.ts` on `:debian: 13 x64-asan` (showing as `[new]` on PRs branched from `f816284bcb1a`). **Root cause:** `enable_macro_mode()` lazily `Box::new`'s a `BufferPrinter` into the `#[thread_local]` `SOURCE_CODE_PRINTER` on whichever bundler worker thread first runs a macro (via `Macro::init` → `VirtualMachine::init` → `enable_macro_mode`). The only free site was `VirtualMachine::deinit`, which the macro VM never reaches (per-worker VM dealloc is intentionally unimplemented). When the worker thread's TLS block is torn down before LSan scans (timing-dependent — debian-13's glibc thread teardown ordering vs Amazon Linux), the 72-byte box is reported. This was always a leak; #30875 unmasked it by removing the `leak:bun_js_parser_jsc::Macro` suppression. Not a #30971 regression. **Fix:** `__bun_macro_context_deinit` already runs on the worker's own thread during `Worker::deinit` and frees the `MacroContext` box (the LSan report showing only 72 B confirms it runs — the much-larger `MacroContext`/`MacroMap`/`bump` arena don't leak). Have it also free the printer when macro mode allocated it. Gated via a `SOURCE_CODE_PRINTER_FROM_MACRO` thread-local so an inline `Bun.build()` macro on the JS thread leaves the runtime VM's printer intact for subsequent module loads. **Tests (debug+ASAN, all 0 fail):** - `test/regression/issue/03830.test.ts` — 1 pass - `test/regression/issue/04893.test.ts` — pass - `test/bundler/transpiler/macro-test.test.ts` — 10 pass - `test/bundler/bun-build-api.test.ts` — 45 pass / 1 skip / 1 todo (covers `Bun.build()` + macros + subsequent imports)
The bundler's
Worker::get(ctx)callsbun_threading::current_thread_id()once per scheduled task to look up the thread'sWorkerin the pool's assignment map. That routes tobun_core::thread_id::current(), which made a freshgettid()/pthread_threadid_np()/GetCurrentThreadId()syscall on every call.A 19K-module bundle (rolldown
apps/10000) schedules ~5.7 tasks per module — parse, line-offset table, quoted source contents, compile-result generation, link step 5 — so it paid ~109,000gettidsyscalls vs ~129 inbun-1.3.14. That was ~36% of the build's total syscall time and a ~15-19% wall-clock regression on the benchmark.Zig's
std.Thread.getCurrentId()doesn't have this cost:LinuxThreadImplreads athreadlocal var tls_thread_idset once at thread start (vendor/zig/lib/std/Thread.zig:841,885). Cache the result in a bare#[thread_local] Cell<ThreadId>slot — same__thread/local-execTLS model as Zig'sthreadlocal var, noLocalKeyinitialization branch or destructor registration. Lazy rather than set-at-spawn so threads not started through Bun's pool (FFI callbacks, the main thread) still get a valid ID.Reproduce: